LT-21988: Improve Hermit Crab performance #275
+277
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JakeOliver28
requested changes
Feb 28, 2025
There was a problem hiding this comment.
Reviewable status: 0 of 14 files reviewed, 3 unresolved discussions
Src/LexText/ParserCore/HCLoader.cs line 354 at r1 (raw file):
{ // Save these classes for later. if (rule == "Clitics")
A switch statement would seem more natural here.
Src/LexText/ParserCore/HCLoader.cs line 434 at r1 (raw file):
// Remove empty strata. foreach (Stratum stratum in m_language.Strata.ToList())
This foreach could be shortened to this:
Code snippet:
m_language.Strata.RemoveAll(stratum =>
!stratum.Entries.Any() &&
!stratum.AffixTemplates.Any() &&
!stratum.MorphologicalRules.Any() &&
!stratum.PhonologicalRules.Any());
// if that's not readable enough, we could also do this
var filterFunction = stratum =>
!stratum.Entries.Any() &&
!stratum.AffixTemplates.Any() &&
!stratum.MorphologicalRules.Any() &&
!stratum.PhonologicalRules.Any();
m_language.Strata.RemoveAll(filterFunction);Src/LexText/ParserCore/HCLoader.cs line 455 at r1 (raw file):
} bool MoveRule(string ruleName, Stratum source, Stratum target)
We could remove redundancy with a helper method here.
Code snippet:
private bool MoveRule(string ruleName, Stratum source, Stratum target)
{
bool found = false;
found |= MoveMatchingItems(source.Entries, target.Entries, entry => m_entryName[entry] == ruleName);
found |= MoveMatchingItems(source.MorphologicalRules, target.MorphologicalRules, rule => rule.Name == ruleName);
found |= MoveMatchingItems(source.PhonologicalRules, target.PhonologicalRules, rule => rule.Name == ruleName);
found |= MoveMatchingItems(source.AffixTemplates, target.AffixTemplates, rule => rule.Name == ruleName);
return found;
}
private bool MoveMatchingItems<T>(IList<T> source, IList<T> target, Func<T, bool> filterFunction)
{
var itemsToMove = source.Where(filterFunction).ToList();
if (itemsToMove.Count == 0) return false;
foreach (var item in itemsToMove)
{
target.Add(item);
source.Remove(item);
}
return true;
}
JakeOliver28
approved these changes
Mar 4, 2025
There was a problem hiding this comment.
Reviewable status: 0 of 14 files reviewed, 3 unresolved discussions (waiting on @jtmaxwell3)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I improve Hermit Crab performance by incorporating a new version of SIL.Machine.HermitCrab which improves the performance of strata and by adding a Strata field to the HC Parser Parameters. The combination improved performance of parsing all of the words in the test project from ~3 hours to ~10 minutes if the Strata field is set to "Templates, [C]ɔ-". How to use the Strata field will have to be documented somewhere.
This change is